Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gravatar Hovercards: jQuery to vanilla JS #15049

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Gravatar Hovercards: jQuery to vanilla JS #15049

merged 1 commit into from
Mar 26, 2020

Conversation

dero
Copy link
Contributor

@dero dero commented Mar 19, 2020

Fixes #14861

Changes proposed in this Pull Request:

  • Rewrite the Gravatar Hovercards to not use jQuery on the front-end.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This is an update to the Gravatar Hovercards module.

Testing instructions:

  • Go to Jetpack > Settings > Discussion.
  • Enable pop-up business cards over commenters’ Gravatars.
  • Enable a theme that displays Gravatars, e.g. twentysixteen.
  • Navigate to a page that displays user avatars, e.g. single post page.
  • Hover over the user gravatar.
  • Observe the business card being displayed.

Proposed changelog entry for your changes:

  • Gravatar Hovercards module no longer depends on jQuery.

@dero dero added [Feature] Gravatar Hovercards [Focus] Performance [Status] Needs Review To request a review from Crew. Label will be renamed soon. DO NOT MERGE don't merge it! labels Mar 19, 2020
@dero dero requested a review from a team as a code owner March 19, 2020 16:45
@dero
Copy link
Contributor Author

dero commented Mar 19, 2020

Assigning to @sgomes who is updating the code over on gravatar.com to not require jQuery. After that effort is done, we'll need to remove the jquery dependency from gravatar-hovercards.php here.

@jetpackbot
Copy link

jetpackbot commented Mar 19, 2020 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 382d77a

@sgomes
Copy link
Contributor

sgomes commented Mar 19, 2020

Assigning to @sgomes who is updating the code over on gravatar.com to not require jQuery. After that effort is done, we'll need to remove the jquery dependency from gravatar-hovercards.php here.

Thanks, @dero! I believe these changes could be merged independently of the Gravatar.com ones, but I don't mind waiting until those are live, as you suggest.

FYI, I completed them earlier today, and they're now going through an internal review process, so hopefully we won't have to wait too long.

@dero
Copy link
Contributor Author

dero commented Mar 19, 2020

Great news, @sgomes!

The changes in Jetpack could be merged, but we would need to keep jquery as a dependency for the module as I believe the script on gravatar.com does not load jQuery if it's not on the page already.

@sgomes
Copy link
Contributor

sgomes commented Mar 19, 2020

The changes in Jetpack could be merged, but we would need to keep jquery as a dependency for the module as I believe the script on gravatar.com does not load jQuery if it's not on the page already.

Yes, that's absolutely right. It does make more sense to do it all in one go later, then, PHP included 👍

@sgomes
Copy link
Contributor

sgomes commented Mar 24, 2020

@dero: There's now a jQuery-less version of gprofiles.js live. The cache-busting param will need to be updated in the PHP file that enqueues it, but that's all it should take.

@dero dero force-pushed the update/gravatar-hovercards-vanilla-js branch from 5719985 to 382d77a Compare March 24, 2020 13:15
@dero
Copy link
Contributor Author

dero commented Mar 24, 2020

Thank you, @sgomes, I've updated the code to not enqueue jQuery + updates the cache buster parameter. Also tested locally and verified that jQuery is not loaded and the hovercards still work. Great work!

@dero dero removed the DO NOT MERGE don't merge it! label Mar 24, 2020
@dero dero mentioned this pull request Mar 24, 2020
Closed
@kraftbj kraftbj added this to the 8.4 milestone Mar 26, 2020
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 26, 2020
@kraftbj kraftbj merged commit fb3824d into master Mar 26, 2020
@kraftbj kraftbj deleted the update/gravatar-hovercards-vanilla-js branch March 26, 2020 22:15
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 26, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
Changelog: add #15049
39736ed
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

Assignees

@sgomes sgomes

Labels
[Feature] Gravatar Hovercards [Focus] Performance
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.

Gravatar Hovercards: Update to not require jQuery
6 participants
@dero @jetpackbot @sgomes @kraftbj @jeherve @matticbot